hi, i did a initial skim and this looks great, its a nice improvement 👍 and def should go insoon

we should spend a bit of time to discuss/prepare for different definitions of suite, as for some people it more important to partition based on a session scoped parameterized fixture for example

however currently xdist does not pass on parameter values and scopes when informing the master of tests - so this may turn out as a unnecessary/bad exercise

Definitely, thanks @carlos-jenkins for the PR.

I will take a closer look ASAP, possibly today.

Also, as the others said: great work and thanks for the PR!

As a follow up on the whole suit thing and Ronny's comment on it. Just to get some random (maybe useless) ideas started, how about considering a command-line option which takes a list of the collection IDs which should be run on one node. That is if you have a structure like pkg0.mod0.TestCls.test_meth etc specifying something like --load_collections=pkg0::mod0,pkg0::mod1,pkg1 would define 3 collections which will not be split up between different nodes. If there are any tests collected which do not have any of the given ID-prefixes then they'll end up anywhere we want (I'd use the lovely standardisation phrase "unspecified" while still guaranteeing they'll be run - but i'm sure people will start relying on whatever implementation we end up choosing first).

we should makr this as experimental and break it if the need arises (reminds me that we should settle experimentation propperly)

That is if you have a structure like pkg0.mod0.TestCls.test_meth etc specifying something like --load_collections=pkg0::mod0,pkg0::mod1,pkg1 would define 3 collections which will not be split up between different nodes. If there are any tests collected which do not have any of the given ID-prefixes then they'll end up anywhere we want

I'm not sure, this kind of information seems to me that it should be defined in the test themselves, not controlled by the command line.

How about we use a mark to group tests, for example @pytest.mark.xdist_group('foobar'), where 'foobar' is any arbitrary string which xdist will use to send tests marked with the same group to a single node? This would allow for arbitrary grouping by any criteria.

Thinking a little further, having the xdist_group mark working would allow one to implement pytest_collect_modifyitems which adds the xdist_group mark with the "suit" of each item, resulting in the same distribution this PR proposes but more general. This would of course need some more changes to the core, at the very least to transfer the xdist_group mark (if any) of each collected item back to the master node so it can properly schedule things.

You're entirely right, markers are a much better way to go about this.

@flub

Seems good at first sight. Main thing I would say is that naming things is hard. pytest never really defined what a suit is and searching the current pytest.org for "suit" seems to suggest it gets used very differently.

Agree. "Suite" has different meanings depending on the context and the person you ask. I'll change the name of everything to loadgroup (?).

@nicoddemus

Thinking a little further, having the xdist_group mark working would allow one to implement pytest_collect_modifyitems which adds the xdist_group mark with the "suit" of each item, resulting in the same distribution this PR proposes but more general. This would of course need some more changes to the core, at the very least to transfer the xdist_group mark (if any) of each collected item back to the master node so it can properly schedule things.

Implementing grouping either using a command line option or markers put a lot of burden to the user.

Consider the options:

1. Command line option:

This will imply to modify the tox file, jenkins file, or whatever triggers the execution of the tests each time a test is added. In our case, we have many colleagues working and adding tests everyday. Not only the command line option will become very very very large, it will become unmaintainable.

2. Markers:

This will also imply that all users must add the @pytest.mark.xdist_group('name_of_module') for each test. Which is not only unnecessary, but can trigger maintainability problems (if for some reason somebody copy pasted, forgot to change, forgot to add it, etc).

In addition, we currently do not pass more than the list of collected tests between nodes, adding introspection to the tests to check if they are marked will imply a large change on the architecture of how xdist currently work, and doesn't add that much value.

So, in order to make it "generic" we are putting a lot of burden to the user.

Let's return back to what we are trying to solve. #18 list 3 use cases:

• Use case: "I want all the tests in a module to go to the same worker."
• Use case: "I want all the tests in a class to go the same worker."

The third I'll discuss it later. But the first two is basically the reuse of fixtures. Currently pytest have 4 scopes for fixtures:

https://docs.pytest.org/en/latest/fixture.html#sharing-a-fixture-across-tests-in-a-module-or-class-session

https://github.com/pytest-dev/pytest/blob/c92760dca8637251eb9e7b9ea4819b32bc0b8042/_pytest/fixtures.py#L848

scope="function"
scope="class"
scope="module"
scope="session"

Let's analyze them one by one:

• session: Grouping by a session fixture makes no sense, as it will be the same as serial.
• function: Grouping by a function fixture is the same as using the load scheduler. So we can mark it as supported.
• module: This scheduler supports it.
• class: This scheduler supports it (read below).

In consequence, grouping by fixture scope is completely supported with this scheduler.

So, what about the third use case?

• Use case: "I want all the tests in X to go to the same worker."

Which is arbitrary grouping, and will then imply markers, that is what you propose. My first impression is, do we really need that? Is that a real use case? For what reasons do we need it, other than hypothetical? Plus #18 suggest a solution:

Solution: You know the drill. If tests belong on the same worker, we are betting that there is an implied, shared dependency. Express that shared dependency as a fixture and pytest will take care of the rest for you.

Which means that you must use a fixture to group them. A scoped fixture, either by class or module, thus basically implying that we need grouping with those scope. Which is what this PR implements.

I added to the example suite the test_delta.py module with two old xunit style classes, and the epsilon package with a single doctest. And collection of nodeids look like this:

example/loadsuite/test/test_alpha.py::test_alpha0
[...]
example/loadsuite/test/test_beta.py::test_beta0
[...]
example/loadsuite/test/test_delta.py::Delta1::test_delta0
[...]
example/loadsuite/test/test_delta.py::Delta2::test_delta0
[...]
example/loadsuite/test/test_gamma.py::test_gamma0
[...]
example/loadsuite/epsilon/__init__.py::epsilon.epsilon

Because this algorithm groups by the first :: Delta1 and Delta2 will be assigned to the same worker. This is a good enough solution as the class fixtures will be reused. Be could also change the algorithm to split not with the first :: but with the last, making classes complete work units.

My 2 cents.

Regards and thanks for comments.

Hi, I just pushed a version with corrections for all the feedback provided, even changing the name to loadscope. In particular I added a function that explains how the nodeids are grouped:

94c1425#diff-13d1cb017e0edf623ce2c3a8a49f9569R278

I changed the split to rsplit and in consequence classes are now scheduled on their own work unit.

With this, I think only two things are missing:

• Tests.
• Changelog and documentation.

Please , let me know if you want to move this implementation forward.

Thanks.

Hi @carlos-jenkins,

First of all thanks for the detailed response. We appreciate both your efforts on this PR as well as your patience on following up in the discussion! 👍

This will also imply that all users must add the @pytest.mark.xdist_group('name_of_module') for each test. Which is not only unnecessary, but can trigger maintainability problems (if for some reason somebody copy pasted, forgot to change, forgot to add it, etc).

You are absolutely right that forcing users to manually keep track of the marking is a huge maintainability problem. I should have mentioned that this marker can be done automatically by a simple hook:

def pytest_collect_modifyitems(items):
    for item in item:
        suite_name = item.module.fspath
        item.add_marker(pytest.mark.xdist_group(suite_name))

The same can easily be extended to group by class as well, or any other way a user needs.

Also, I'm not suggesting that users should implement this hook themselves, but by implementing the distribution scheduling in a more general way we can provide builtin options in xdist which use the idea of the hook above to set markers to items automatically, while allowing users to implement more complex scheduling themselves if they want to.

For example, we could create a command-line option which controls how it should group tests by default:

• --group-by=no: no automatic grouping (default);
• --group-by=module: automatically sets the xdist_group mark to the module name of the test item;
• --group-by=class: automatically sets the xdist_group mark to the class name of the test item;

and so on.

In addition, we currently do not pass more than the list of collected tests between nodes, adding introspection to the tests to check if they are marked will imply a large change on the architecture of how xdist currently work

Indeed we would have to change some things on the architecture, but unless I'm mistaken they would be minimal changes: currently the workers send just a list of item ids to the master; to be able to handle the marker idea, we would have to send in addition to that a dict of "test id -> marker group name". The workers can easily obtain the value of the xdist_group marker after collection and before sending to master.

Now that I think more about it, we don't even need a separate scheduler, we might be able to change the default LoadScheduler to handle the markers idea. This would avoid having to duplicate some code.

session: Grouping by a session fixture makes no sense, as it will be the same as serial.

This is only true of the session fixture is used by all the tests, like an autouse fixture.

Here's an use case based on a session-scoped fixture:

Suppose you have a session fixture which is very costly to create (it starts up a server inside a dock container) but this session fixture is only used by a small portion of your tests (say 10%). In the current state of things, if you spawn 8 workers chances are that you will endup spinning 8 of those fixtures because the tests which use this fixture will most likely go to separate workers.

With the xdist_group mark idea, the user could implement a hook like this:

def pytest_collect_modifyitems(items):
    for item in item:
        if 'docker_server' in item.fixturenames:
            item.add_marker(pytest.mark.xdist_group('docker-server'))

So all tests which use this fixture (whenever they are defined) will go to the same worker, spinning up a single server.

Another example:

In a test suite a few tests (8 out of 400) take a very long time (8+ minutes). Those slow tests don't have much in common, they are slow because they are running some numerical simulations which due to their parameters do take a lot of time. When we initially put them in xdist, unfortunately it happened that a lot of those tests would end up in the same worker. For example, it might happen that 6 of those tests to land on the same worker because xdist currently is not deterministic when deciding which test goes where, so a single worker might end up taking 48min (8x6) to finish, meanwhile the other workers will have finished the tests long after that.

By marking them with a custom mark (say @pytest.mark.very_slow), we can assign groups to those tests in a round robin fashion.

def pytest_collect_modifyitems(items):
    numprocesses = items[0].config.numprocesses
    current_worker = 0
    for item in item:
        m = item.get_marker('very_slow')
        if m:
            item.add_marker(pytest.mark.xdist_group(f'worker-{current_worker}'))
            current_worker += 1
            if current_worker >= numprocesses:
                current_worker = 0

This will distribute evenly the slow tests across the available workers.

The examples above all came from work, and we had to implement ugly hacks to get around them.

@carlos-jenkins I hope I'm not coming across as trying to shoot down your efforts, I'm just trying to brainstorm with everyone involved here to see if perhaps we can get to a better/more general implementation.

Guys,

If we can't get to a consensus now (I mean everyone involved in the discussion), I would rather us work towards merging this PR as it stands because it will improve how things work currently. Also, I think it would be possible to implement this idea of using markers afterwards with some internal refactoring and also without breaking the behavior being introduced here.

Again @carlos-jenkins thanks for your patience!

@carlos-jenkins thanks for your updates, in near future i#d like to get interested people together for changing the shape

currently imalready preparing to shed xdist from boxed/looponfails and setting up more comprehensible statemachines for xidst, afterwards i'd like to extend sheduling for per file, per fixture and similar details

Hello, somehow I don't find this outcome very satisfying. Here we had code that works right now and solves a sub-set of the problem, so can't we find a way to use it? We all have grand plans for many things but realistically also know they only get realised rarely.

If we where to add this code as-is to the core now we'd have a new scheduler type. We'd just need to be a little careful about naming it but it would work just fine. It is my understanding that if we then later add a mark-scheduler we do not have to do anything else and there would be no conflict. So I'm not actually sure there's any practical downside to adding this now at all. Did I overlook something?

An alternative approach which was mentioned I think and may not be too much extra work is to turn this scheduler into a plugin. I'd then update the standard documentation to point to this plugin and it would would be a great example for other people who want to customise their own scheduling. Again I've not looked into the practicalities of this, but what are the changes needed to turn this into a plugin?

Cheers

Hello, somehow I don't find this outcome very satisfying.

I agree, it was not my intention to just shut down the PR at all.

I believe we can add this to the core and refactor things later to support the mark-based distribution idea.

Creating a separate plugin with this functionality is also possible I think.

(Oops clicked on the wrong button)

then lets flag it as experimental and merge ? @carlos-jenkins would you be ok with such an integration?

@RonnyPfannschmidt sure, for me this is fine. We can pin the hash of the merge in our requirements.txt so you feel free to change anything without breaking us.

Thanks @carlos-jenkins

Before release, how about we create a separate option --group-by, which is used when --dist is load? By default, group-by would be none which sends tests individually to workers, and group-by=module provides the functionality of this PR. Having a separate option would allow us to extend the option to support grouping by classes in the future.

So this:

pytest --dist=loadsuite ...

Becomes:

pytest --dist=load --group-by=module ...

What do you guys think?

I think that is perfect @nicoddemus. Just a detail, the current implementation groups by the last :: in the tests ids, so it will group modules of functions, or classes if modules of classes:

b0916f0#diff-13d1cb017e0edf623ce2c3a8a49f9569R278

since this works right now i propose merging first as an experimental feature, then pushing changes on top

I think we should at least add some functional tests and a changelog entry presenting the feature before merging.

Just a detail, the current implementation groups by the last :: in the tests ids, so it will group modules of functions, or classes if modules of classes:

Oh right. Hmm not sure how to name it then, --group-by=modules-and-classes? 😁

Or we can leave as --group-by=suite if people think that's better.

But as @RonnyPfannschmidt mentioned, we can leave the bikeshedding of the option name to later.

I'm not very keen on this. It makes more assumptions that the next
scheduler feature will want different kind of grouping. If we do this as
just a new scheduler name, e.g. --dist=loadbunch or so, then we don't
introduce anything new to any other part of xdist. If we later add another
grouping and it still makes sense to add --group-by then we can add it at
that point.

No problem in adding this later, I guess it is acceptable we break the UI later for an experimental feature. 👍

@flub you are right, a separate --dist option doesn't necessarily map to a separate Schedular implementation, the same implementation can handle multiple --dist=loadxxx options. 👍

Hi guys,

I'm writing an acceptance test for this feature so we can get it in, but I'm not getting the results I was expecting.

Consider 3 test files, test_foo.py, test_foobar.py and test_bar.py, with the same contents:

import pytest
@pytest.mark.parametrize('i', range(10))
def test(i):
    pass

Running:

pytest --dist=loadscope -n2 -v

I get this output:

============================= test session starts =============================
platform win32 -- Python 3.6.0, pytest-3.1.2, py-1.4.34, pluggy-0.4.0 -- x:\pytest-xdist\.env36\scripts\python.exe
cachedir: ..\.cache
rootdir: X:\pytest-xdist, inifile: tox.ini
plugins: xdist-1.18.2.dev7+g144b37a.d20170727
[gw0] win32 Python 3.6.0 cwd: X:\pytest-xdist\.tmp
[gw1] win32 Python 3.6.0 cwd: X:\pytest-xdist\.tmp
[gw0] Python 3.6.0 (v3.6.0:41df79263a11, Dec 23 2016, 08:06:12) [MSC v.1900 64 bit (AMD64)]
[gw1] Python 3.6.0 (v3.6.0:41df79263a11, Dec 23 2016, 08:06:12) [MSC v.1900 64 bit (AMD64)]
gw0 [30] / gw1 [30]
scheduling tests via LoadScheduling

test_bar.py::test[1]
test_bar.py::test[0]
[gw1] PASSED test_bar.py::test[1]
[gw0] PASSED test_bar.py::test[0]
test_bar.py::test[3]
test_bar.py::test[2]
[gw1] PASSED test_bar.py::test[3]
[gw0] PASSED test_bar.py::test[2]
test_bar.py::test[4]
[gw0] PASSED test_bar.py::test[4]
test_bar.py::test[5]
[gw1] PASSED test_bar.py::test[5]
...
(snip the rest)

As can be seen, tests from test_bar.py are running in both gw0 and gw1. I was expecting for all tests from each module to run always in the same worker. Am I missing something? I will investigate this a little more when I have some time.

Hi @nicoddemus

Thanks for the effort on testing this.

I've noted that you're using -n2 which immediately implies using the LoadScheduling. You can actually read in the output:

scheduling tests via LoadScheduling

The correct way to call it is with:

pytest  --dist=loadscope --tx=2*popen -v

You should be able to read:

scheduling tests via LoadScopeScheduling

Rá, indeed! Thanks!

I will push my tests to your branch soon. 😁

Pushed two very basic functional tests for --dist=loadscope, and also made -nX and --dist=loadscope work properly together.

I didn't add any docs yet.

If you guys are OK with the implementation so far I guess we can merge it now that we have some regression tests in place.

@nicoddemus thanks for tieing up @carlos-jenkins fabulous work

we now need to decide how to document this as experimental and how to communicate/implement eventual changes on it